NPM - Esm modules are not supported, dynamic (async) import fails

[crutch12]

Bug Description

Nowadays most of npm packages are published with esm files only.
So you can't require these modules, you should import them.

const urlJoin = require('url-join')
// Error: require() of ES Module /app/available_modules/1716112470000/url-join/lib/url-join.js from /app/index.js not supported.
// Instead change the require of url-join.js in /app/index.js to a dynamic import() which is available in all CommonJS modules.

For example: url-join

But import is not available in n8n node:

// lodash example
import lodash from 'lodash'
// ERROR: 'import' and 'export' may only appear at the top level [line 1]

And dynamic import is not available too:

// lodash example
const lodash = await import('lodash')
// ERROR: VMError: Dynamic Import not supported [line 1]

To Reproduce

1. Create n8n workflow
2. Create Code node (js)
3. Add import lodash from 'lodash' or const lodash = await import('lodash')
4. Run Test step

Expected behavior

import('lodash') should return Promise

Operating System

Ubuntu Linux 22.04

n8n Version

1.44.1

Node.js Version

18.20.2

Database

SQLite (default)

Execution mode

main (default)

[Joffcom]

Hey @crutch12

I believe this could be a limitation of the JavaScript sandbox we use.

@netroy may be able to confirm this.

[crutch12]

Hey @Joffcom

Talking about JavaScript sandbox

You use https://github.com/patriksimek/vm2
But it has attention:

‼️ Project Discontinued ‼️
TL;DR The library contains critical security issues and should not be used for production! The maintenance of the project has been discontinued. Consider migrating your code to isolated-vm.

Why don't you use isolated-vm instead?

With NODE_FUNCTION_ALLOW_BUILTIN=* I can do terrible things:

const { execSync } = require('child_process');

console.log(Buffer.from(execSync('pwd')).toString()) // it works

console.log(Buffer.from(execSync('npm install axios')).toString()) // it works

console.log(Buffer.from(execSync('env')).toString()) // it works (!)

console.log(Buffer.from(execSync('ps')).toString()) // it works (!)

// DO NOT RUN IT
// console.log(Buffer.from(execSync('rm -rf /usr/local/lib/node_modules/n8n')).toString()) // it works (!)

So any n8n user may do anything in n8n contrainer... I thought sandboxes should prevent this behaviour 😕

[Joffcom]

Hey @crutch12,

We currently use our own fork of vm which has some fixes for the issues in it but it is isn't perfect, We looked at moving to other options which involve a lot of work to implement. If you look at the open discontinued issue on vm2 you will find some of our team in there, We are currently evaluating different options to see which fits our needs more.

While not perfect in terms of risk for what you have provided not only do you need access to the n8n interface you would also need the server admin to have enabled all builtin functions. A much easier way to do the same set of commands would be to get access to the workflow builder and use the execute command node, A way to prevent this would be to set NODES_EXCLUDE to execute the code and execute command nodes.

However this thread is not the place for the vm2 sandbox escape issue and if you want to raise your concern with the security team you can pop in an email to security@n8n.io as outlined here: https://github.com/n8n-io/n8n/blob/master/SECURITY

[netroy]

as Jon pointed out, not being able to use ESM is a limitation of vm2.

You use https://github.com/patriksimek/vm2 But it has attention:

‼️ Project Discontinued ‼️
TL;DR The library contains critical security issues and should not be used for production! The maintenance of the project has been discontinued. Consider migrating your code to isolated-vm.

We are aware. If you look at some their discussions, you'll see that we've been part of them.
This is why we maintain our own fork, where we have fixed as many of the known vulnerabilities as possible.
This is something we really want to address, but this is a much bigger task than one might think at a first glance.

Why don't you use isolated-vm instead?

vm2 suggests that people migrate to isolated-vm because there aren't that many mature sandboxing options. However, isolated-vm is not a drop-in replacement.

If I could make this decision at the very beginning on n8n, I'd have gone for isolated-vm.
But we can't do that now because

1. isolated-vm uses V8 isolates, and has none of the nodejs/commonjs APIs. This means that we can't use any node modules without explicitly bundling the entire dependency tree (including nodejs native packages) into the JS that gets executed in the isolate. To avoid breaking every workflow out there, we have to keep nodejs/commonjs APIs available and backward-compatible inside the sandbox.
2. V8 isolates do not make it easy to proxy input/output data which almost all n8n workflows need. We could try to build complicated harnesses that copy data and out of the isolate using something like structuredClone, but that would increase the memory usage of the code node significantly.

Even if we were to put significant effort to pull this off somehow, isolated-vm is being barely maintained, and the maintainers have said they will not be adding any new features.
We don't know when they might get hit by a security vulnerability that is so much effort to fix that the maintainers rather abandon the project (like vm2 had to).
I'm not trying to talk down the maintainers of these projects. Creating secure sandboxes is a very difficult problem, and it is understandable that sooner or later most people working on such projects give up playing the game of whac-a-mole that securing sandboxes is.

With NODE_FUNCTION_ALLOW_BUILTIN=* I can do terrible things:

1. You don't need the Code node to do those horrible things. You can execute arbitrary commands via the ExecuteCommand node directly.
2. NODE_FUNCTION_ALLOW_BUILTIN defaults to '', which means no nodejs modules are available in the sandbox by default. If you have the permissions to change NODE_FUNCTION_ALLOW_BUILTIN to *, then you already have permissions to much more damage, and this should be the least of concerns for the person managing the n8n instance.

So, unless we decide to create our own new sandboxing mechanism, or rewrite the entire execution engine on top of a completely different runtime, we (unfortunately) can't really address any of your concerns in this issue, and should close this.

If security is really important to you, I'd highly recommend setting NODES_EXCLUDE env variable to [\"n8n-nodes-base.code\", \"n8n-nodes-base.executeCommand\"] to prevent arbitrary code or command execution.

[crutch12]

@netroy thank you for detailed answer!

then you already have permissions to much more damage

Yes, I have. But I talk about another users in my n8n self-hosted instance. Or about npm malware packages.

BTW

Have you seen Pipedream? (https://pipedream.com/)
It allows you to create separated Code nodes, use import statements. Every node is independent.
It also automatically installs packages!

I think Pipedream runs every Code node in independent sub VM/sandbox and it works really great!

Maybe we could check out how they do it?
https://github.com/PipedreamHQ/pipedream

[Joffcom]

Hey @crutch12,

Pipedream don't make their runtime available like we do so what they do is hidden, It is possible that the approach they use also has issues that have just not been reported yet or maybe as it is all running in the cloud they start up a new docker image and pass the code to run to that image and remove it once completed.

If you have multiple users and you are worried about the security of your instance we would recommend not allowing access to all node functions which is what we do on our Cloud service as well as blocking access to the execute command node, When it comes to npm malware packages unless you are manually adding them to the n8n image there should be a very minimal risk there as standard users can't install packages (unless you allow access to some node functions or allow the execute command node to be used).

As the original topic here has been answered and is down to a limitation in the package and it is something we don't consider to be a bug at the moment I am going to mark this as closed. If / When we change the sandbox approach though I will make sure we pop a note back on this to let you know.